-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
QA Team #45
base: master
Are you sure you want to change the base?
QA Team #45
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on this first draft, @j01tz! See comments from me
text/0000-qa-team.md
Outdated
* Before submitting the PR, the contributor reads and integrates the well-formed contribution documentation to assist and support their efforts | ||
* Before the PR is accepted the contributor may use the Grin testing harness framework provided by the QA team. | ||
* Members of the QA team may manually review to changes for quality at a micro level: does the code do a good job of implementing the feature, is it readable, are there any obvious security issues, does it follow our contribution guidelines for formatting etc. | ||
* The QA may also review the change for quality at a macro level: are the changes in line with the other pieces of the project, does it add inconsistencies when interacting with other parts, does it advance the expected overall quality of the project etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as previous comment here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise, I clarified the example, let me know if it seems more appropriate now.
Thanks for your comments @lehnberg. I updated with your feedback and some clarifications, let me know what can be improved 👍 |
Nice work, @j01tz - I think this looks pretty good personally, might be good enough to move out of draft and let others review in more detail |
|
||
## Example | ||
|
||
In this example a large PR is submitted that makes a nontrivial change to the Grin node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering to what extent non-developer members of the QA team will be expected to fully understand the implications of major changes, particularly to the point where they're expected to help review PRs. Is it more a case of the QA team ensuring that tests exist or at least a testing strategy exists for verifying changes in PRs rather than the entire substance of the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this point is important to discuss as test coverage can be a bit subjective and the inability to fully understand the changes could result in QA approving coverage of a feature that isn't properly tested. My initial reaction is that the QA team is there to stand up processes, protocols and tools and ensure that they are properly used by contributors. As indicated, development knowledge is required to assess whether the test coverage is accurate (not to mention long term implications arising from the relevant changes that are not realizable with testing frameworks alone).
We may decide that it is sufficient for a QA team to ensure that changes are trivially covered by the testing framework and that other tests don't fail. We may decide that there is no point in a QA team if it cannot make a more meaningful assessment than a binary test pass/fail, covered/not. It could be helpful to distinguish what is required for a minimally viable QA team that adds tangible value and what is required for an ideal high-performing QA team.
# Unresolved questions | ||
[unresolved-questions]: #unresolved-questions | ||
|
||
* To what extent does the QA team develop tools? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part of it is key to me. In my mind an effective QA takes a very active role in the creation of tools, frameworks and environments that continually deploy and test changes. Of course this can be supported by developers, but the QA team very much needs to drive the overall testing vision and take the leadership to ensure the correct tools are in place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be reasonable to expect the QA team to develop and make available the tools for sufficient testing. This looks like a big initial effort to get a test harness to support the node, wallet and miner. Additionally if new aspects emerge that require integration, an initial effort may be required to glue the necessary pieces so that contributors can begin adding and testing those as well. Beyond those initial efforts to add support to unsupported areas, as currently written, any changes should really be the responsibility of contributors.
However if the QA team doesn't give contributors everything possible to succeed (good documentation, sane frameworks, stationary goalposts etc.) then the responsibility comes back to the QA team in my opinion. There's probably room for a lot of discussion and clarification here. For an ideal QA team, technical vision and leadership is essential. Whether this is necessary for a minimally viable QA team seems open (as is the question of whether a minimally viable QA team is worth it).
|
||
## Community Contributions | ||
|
||
The QA team is ultimately responsible for creating an environment that encourages and empowers contributors to make quality contributions to Grin. To this end, the QA team will strive to provide the resources necessary for contributors to make consistent, high quality contributions to Grin, including but not limited to: documentation, constructive feedback, tooling and clarity of expectations of quality. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a little bit at odds with what's stated above about what the responsibilities do not include... for instance, surely the QA team will want to ensure their tooling and testing frameworks are up to date? Perhaps they won't be expected to maintain them 100% themselves, but the team would be the one striving to ensure they're in place, up-to-date and effective
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it is currently written, my understanding is that the QA team is responsible for ensuring the tooling and frameworks are working before a contributor starts (this should be a function of the initial setup and ensuring that contributors don't make changes without adding working test coverage).
The distinction is that the QA team is not responsible for updating the test framework to cover a change made by a contributor- the QA team is responsible for making sure that the contributor updates the test framework to cover their own change. This is supported by the QA team ensuring that expectations are documented, that the initial framework was set up properly and that changes from there were covered by their contributors.
@j01tz Thanks for putting this together, I think it's a good start. A few comments above. My overall impression is that I think it needs to be more assertive about the role the QA team is intended to play. I don't think it will be much use to just have a team dedicated to another level of review on PRs or just point out when something isn't being covered by automated testing; Overall I think an effective QA team needs to have a very active presence in the entire development process, and be very assertive about defining tooling, environments, test strategy, and working with development teams to ensure this is all in place. Some of the roles and responsibilities above lead me to think that perhaps there's a bit of discussion needs to be had about how technical the QA team is expected to be. In my mind a good QA team isn't just a bunch of people who run through test scripts thrown over the wall to them by a development team, but should also contain a decent engineering element who understand the product and aren't afraid to own the tooling and frameworks. |
Thanks for the review @yeastplume
I agree with this in general. One of the objectives in writing this was to be more descriptive rather than prescriptive. I want to describe the team and what it should look like. Once the QA team is formed the hope is that members update this RFC with more prescriptive language about what they should be doing/providing explicitly and how they should do it. This allows a working QA team to form the strategy they deem most likely to provide quality assurance to the project as a whole. I am not convinced this is 100% the right approach necessarily, it is just the one I have taken so far- maybe we as a community do want to be both prescriptive and descriptive when starting the QA team.
As mentioned in a previous comment, it could be helpful for us to distinguish what is minimally expected for a realistic functional QA team that adds tangible value to the project and what would be expected for an ideal efficient and performant QA team. Maybe the technical balance requirements of the QA team will come out in this conversation. I'm sure @lehnberg has opinions about all of the above as well. |
Rendered link to RFC document
This is a proposal to create a QA team for Grin.